Skip to content

[blockchain] Add traits to reuse Blockchains across multiple wallets #569

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
May 11, 2022
Merged

[blockchain] Add traits to reuse Blockchains across multiple wallets #569

merged 3 commits into from
May 11, 2022

Conversation

afilini
Copy link
Member

@afilini afilini commented Mar 15, 2022 •
edited
Loading

Description

Add three new traits:

  • StatelessBlockchain is used to tag Blockchains that don't have any wallet-specic state, i.e. they can be used as-is to sync multiple wallets.
  • StatefulBlockchain is the opposite of StatelessBlockchain: it provides a method to "clone" a Blockchain with an updated internal state (a new wallet checksum and, optionally, a different number of blocks to skip from genesis). Potentially this also allows reusing the underlying connection on Blockchain types that support it.
  • MultiBlockchain is a generalization of this concept: it's implemented automatically for every type that implements StatefulBlockchain and for every Arc<T> where T is a StatelessBlockchain. This allows a piece of code that deals with multiple sub-wallets to just get a &B: MultiBlockchain without having to deal with stateful and statless blockchains individually.

These new traits have been implemented for Electrum, Esplora and RPC (the first two being stateless and the latter stateful). It hasn't been implemented on the CBF blockchain, because I don't think it would work in its current form (it throws away old block filters, so it's hard to go back and rescan).

This is the first step for #549, as BIP47 needs to sync many different descriptors internally.

It's also very useful for bitcoindevkit/bdk_wallet#188.

Notes to the reviewers

This is still a draft because:

  • I'm still wondering if these traits should "inherit" from Blockchain instead of the less-restrictive WalletSync + GetHeight which is the bare minimum to sync a wallet
  • I need to write tests, at least for rpc which is stateful
  • I need to add examples

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

@afilini afilini marked this pull request as draft March 15, 2022 10:07
@afilini afilini mentioned this pull request Mar 21, 2022
Closed
15 tasks
@afilini afilini force-pushed the features/multi-blockchain branch 3 times, most recently from 6a88c88 to ae9c01d Compare March 23, 2022 13:51
@afilini
Copy link
Member Author

afilini commented Mar 23, 2022

I didn't like how StatefulBlockchain worked, so I just removed that trait since it was very similar to MultiBlockchain anyway. I've also renamed MultiBlockchain to BlockchainFactory (@notmandatory used this term in yesterday's call and I really liked it).

I've also updated all these traits to inherit from Blockchain rather than GetHeight + WalletSync: the rationale is that it's easier to relax these requirements later rather than tightening them as it would be an API break.

I've added tests and documentation, I'm feeling confident to remove the draft state from this PR.

@afilini afilini marked this pull request as ready for review March 23, 2022 13:52
@afilini afilini force-pushed the features/multi-blockchain branch 2 times, most recently from e010c1c to a0cf1f6 Compare March 23, 2022 14:09
notmandatory
notmandatory reviewed Mar 26, 2022
View reviewed changes
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. I commented on some fixes needed in the rpc docs code, and a couple other suggestions.

@afilini afilini force-pushed the features/multi-blockchain branch 5 times, most recently from 06159e1 to a817545 Compare April 13, 2022 14:54
rajarshimaitra
rajarshimaitra suggested changes Apr 15, 2022
View reviewed changes
Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK a817545

I think this would be really useful for multiple wallets to use the same backend.

Below are some nits and api questions I had..

@afilini afilini force-pushed the features/multi-blockchain branch 2 times, most recently from dc53609 to 0657126 Compare April 15, 2022 20:13
@afilini
Copy link
Member Author

afilini commented Apr 15, 2022

Rebased and addresses most comments

@afilini afilini force-pushed the features/multi-blockchain branch 3 times, most recently from de1ab13 to 6f91598 Compare April 15, 2022 20:37
rajarshimaitra
rajarshimaitra approved these changes Apr 29, 2022
View reviewed changes
Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 6f91598

Code changes looks good to me. Now I understand the intention better. This will be really useful..

Just one non blocking nit. Can be done in a following PR.

@notmandatory
Copy link
Member

This one needs one more rebase to pickup the new MSRV CI changes and then it should be ready to go.

@afilini
Move testutils macro module before the others
2d83af4 
This allows using the `testuitils` macro in their tests as well
@afilini afilini force-pushed the features/multi-blockchain branch from 6f91598 to 6031e23 Compare May 9, 2022 17:31
afilini added 2 commits May 9, 2022 19:34
@afilini
[blockchain] Add traits to reuse Blockchains across multiple wallets
9c405e9 
Add two new traits:
- `StatelessBlockchain` is used to tag `Blockchain`s that don't have any
  wallet-specic state, i.e. they can be used as-is to sync multiple wallets.
- `BlockchainFactory` is a trait for objects that can build multiple
  blockchains for different descriptors. It's implemented automatically
  for every `Arc<T>` where `T` is a `StatelessBlockchain`. This allows a
  piece of code that deals with multiple sub-wallets to just get a
  `&B: BlockchainFactory` to sync all of them.

These new traits have been implemented for Electrum, Esplora and RPC
(the first two being stateless and the latter having a dedicated
`RpcBlockchainFactory` struct). It hasn't been implemented on the CBF
blockchain, because I don't think it would work in its current form
(it throws away old block filters, so it's hard to go back and rescan).

This is the first step for bitcoindevkit#549, as BIP47 needs to sync many different
descriptors internally.

It's also very useful for #486.
@afilini
wallet: Move wallet_name_from_descriptor above the tests
8795da4
@afilini afilini force-pushed the features/multi-blockchain branch from 6031e23 to 8795da4 Compare May 9, 2022 17:34
rajarshimaitra
rajarshimaitra approved these changes May 10, 2022
View reviewed changes
Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 8795da4

@notmandatory notmandatory merged commit 7aa2746 into bitcoindevkit:master May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@notmandatory notmandatory notmandatory left review comments

@rajarshimaitra rajarshimaitra rajarshimaitra approved these changes

Assignees

@afilini afilini

Labels
None yet
Projects
None yet
Milestone
Release 0.19.0 Feature Freeze
Development

Successfully merging this pull request may close these issues.
3 participants
@afilini @notmandatory @rajarshimaitra